-
Notifications
You must be signed in to change notification settings - Fork 1k
Prototype of login via prototyper in Studio #8938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @joehan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new, non-interactive login flow specifically designed for the Studio prototyper environment. It extends the existing authentication module with a programmatic way to initiate and complete user login, bypassing traditional interactive prompts and reauthentication checks when enabled.
Highlights
- New Non-Interactive Login Flow: I've introduced a new
loginPrototyper
function insrc/auth.ts
. This function facilitates a non-interactive login flow, returning a URI for the user to initiate authentication and anauthorize
method to complete the process with an authorization code. This is designed for environments like the Studio prototyper where direct user interaction with the CLI for login is not feasible. - Integration with
login
Command: Thelogin
command insrc/commands/login.ts
has been updated to accept a newprototyperLogin
option. When this option is provided, the command delegates to the newloginPrototyper
function, enabling the non-interactive flow. This allows programmatic login via thefirebase-tools
module. - Bypassing Interactive Checks: To support the non-interactive nature of the prototyper login, I've modified the
login
command's logic to bypass the standardnonInteractive
error and thereauth
check when theprototyperLogin
option is active. This ensures the flow can proceed without requiring user prompts or checking for existing credentials.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a prototype for a non-interactive login flow for Studio. The changes are well-contained, adding a new loginPrototyper
function and integrating it into the existing login
command. My review focuses on improving error handling and robustness, and I've identified a critical issue with command-line option processing that could affect the default login behavior. Overall, this is a good starting point for the new feature.
// Default to using the authorization code flow when the end | ||
// user is within a cloud-based environment, and therefore, | ||
// the authorization callback couldn't redirect to localhost. | ||
const useLocalhost = isCloudEnvironment() ? false : options.localhost; | ||
|
||
const useLocalhost = isCloudEnvironment() ? false : !!options.localhost; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for determining useLocalhost
appears to be incorrect. The --no-localhost
option implies that using localhost is the default behavior (true
). If options.localhost
is undefined
when the flag is not provided, !!options.localhost
evaluates to false
, which is the opposite of the intended default. This would break the standard login flow for users not in a cloud environment.
const useLocalhost = isCloudEnvironment() ? false : !!options.localhost; | |
const useLocalhost = isCloudEnvironment() ? false : options.localhost !== false; |
const attestToken = ( | ||
await authProxyClient.post<{ session_id: string }, { token: string }>("/attest", { | ||
session_id: sessionId, | ||
}) | ||
).body?.token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation doesn't handle the case where authProxyClient.post
returns a response without a body or a body without a token. This would result in attestToken
being undefined
, which would then be embedded in the loginUrl
, likely breaking the authentication flow.
Please add a check for the token and throw an error if it's missing.
const attestToken = ( | |
await authProxyClient.post<{ session_id: string }, { token: string }>("/attest", { | |
session_id: sessionId, | |
}) | |
).body?.token; | |
const attestRes = await authProxyClient.post<{ session_id: string }, { token: string }>("/attest", { | |
session_id: sessionId, | |
}); | |
const attestToken = attestRes.body?.token; | |
if (!attestToken) { | |
throw new FirebaseError("Failed to get attestation token, please try again."); | |
} |
src/auth.ts
Outdated
return { | ||
user: jwt.decode(tokens.id_token!, { json: true }) as any as User, | ||
tokens: tokens, | ||
scopes: SCOPES, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jwt.decode
function can return null
or a string
, and tokens.id_token
can be undefined
. The current code uses a non-null assertion !
and a broad type cast as any as User
, which can lead to runtime errors or incorrect data being returned if the token is invalid or missing. It's safer to handle these cases explicitly to ensure type safety and prevent unexpected runtime failures.
const decodedUser = jwt.decode(tokens.id_token || "", { json: true });
if (!decodedUser || typeof decodedUser === "string") {
throw new FirebaseError("Authentication failed: could not decode user token.");
}
return {
user: decodedUser as User,
tokens: tokens,
scopes: SCOPES,
};
} catch (e) { | ||
throw new FirebaseError( | ||
"Unable to authenticate using the provided code. Please try again.", | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The catch
block currently swallows the original error, which can make debugging difficult if an unexpected issue occurs during authorization. It would be helpful to log the original error for better diagnostics.
} catch (e: unknown) {
logger.debug("Prototyper login authorization failed:", e);
throw new FirebaseError(
"Unable to authenticate using the provided code. Please try again.",
);
}
Description
Early prototype of a noninteractive login flow for use in Studio prototyper.
Using it
Roughly, we'll do something like: